-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for LLVM globals corresponding to miri allocations should be named alloc123 #69155
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I think that everything that has been raised so far is addressed here other than Oliver's recommendation:
Let me know if there is anything else that you need from me. |
|
Ready for review |
src/test/codegen/consts.rs
Outdated
|
||
// This checks the constants from {low,high}_align_const, they share the same | ||
// constant, but the alignment differs, so the higher one should be used | ||
// CHECK: [[LOW_HIGH:@[0-9]+]] = {{.*}} getelementptr inbounds (<{ [8 x i8] }>, <{ [8 x i8] }>* @2, i32 0, i32 0, i32 0), {{.*}}, | ||
// CHECK: [[LOW_HIGH:@[0-9]+]] = {{.*}} getelementptr inbounds (<{ [8 x i8] }>, <{ [8 x i8] }>* @alloc15, i32 0, i32 0, i32 0), {{.*}}, align 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing, but @alloc15
's definition doesn't seem to exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything actionable here? I don't understand this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a // CHECK
comment for @alloc15
, presumably, but I'm unsure why it doesn't exist.
src/test/codegen/consts.rs
Outdated
@@ -10,11 +10,11 @@ | |||
// CHECK: @STATIC = {{.*}}, align 4 | |||
|
|||
// This checks the constants from inline_enum_const | |||
// CHECK: @{{[0-9]+}} = {{.*}}, align 2 | |||
// CHECK: @alloc5 = {{.*}} zeroinitializer, align 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized the zeroinitializer
shouldn't be here either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me after fixing the remaining nit and squashing commits
4b2ee45
to
cf929f7
Compare
squashed to cf929f7 and force pushed should be gtg |
@bors r+ rollup=never (we don't know if it will succeed on macOS) |
📌 Commit cf929f7 has been approved by |
@bors p=1 |
☀️ Test successful - checks-azure |
Adds support for this request from @eddyb in #69134:
r? @eddyb
cc @oli-obk